-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor and add unit tests for operator reconciler #115
refactor and add unit tests for operator reconciler #115
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make test-unit
locally succeeds
make e2e
locally succeeds
But make test
locally fails (even after a rebase to main
). It passes on main
, however.
(make test
should apparently be part of the CI)
It's actually failing on make test-e2e
...
if err != nil { | ||
status = metav1.ConditionTrue | ||
status = metav1.ConditionFalse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this remain ConditionTrue
as resolution did fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConditionTrue
is saying Ready: True
. Since resolution failed, shouldn't we be saying Ready: false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, I think the structure of how these condition values gets set makes it difficult to reason about. I've fixed this in #107, so as soon as this change lands, I can layer in another PR that simplifies the Reconcile
function and makes it more obvious how/when the ready condition is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I was conflating "Resolution failed" with the type.
OK, I think the confusion is that it's not obvious this is for the "Ready" type, since TypeReady
is not very local, it's down 30+ lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ Exactly - That problem will be resolved in #118.
// to ensure that exec-entrypoint and run can make use of them. | ||
_ "k8s.io/client-go/plugin/pkg/client/auth" | ||
|
||
rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering why this moved up here (i.e. away from the other github.com
imports). I have seen go fmt
move things around, but it seems fine with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, that my IDE running gosimports
(https://github.com/rinchsan/gosimports) to automatically organize imports into groups and sort them.
Re:
|
b91ac8c
to
52bbccb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
By("running reconcile") | ||
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey}) | ||
Expect(res).To(Equal(ctrl.Result{})) | ||
// TODO: should resolution failure return an error? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably trigger reconcile again with the error that we are setting in the condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, I've got that fixed up in #118. When this merges, I'll rebase that one.
Expect(cond).NotTo(BeNil()) | ||
Expect(cond.Status).To(Equal(metav1.ConditionTrue)) | ||
Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonResolutionSucceeded)) | ||
Expect(cond.Message).To(Equal("resolution was successful")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not related to the PR, but just noticed) We should probably define separate error types and constants for the messages to make it easier for users to check the status message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed to constants for messages we reuse, but I'm pretty sure condition messages are not considered stable or part of the API a consumer is supposed to depend on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Joe. Reason
is the programatic interface.
But using constants for messages is not a bad idea, either.
See #119 |
@tmshort anything else you want to review/discuss on this one? |
Not concerned about the apidiff failure. While we're in development, the purpose of that is to inform us of the changes we're making to the public API, and potentially have discussion about it. Honestly, the main thing I'm watching for at this stage is unnecessarily adding new surface area to the public API. Not really at all concerns about removals or changes at this point. |
Another facet of #107, covering the following:
added more unit tests to increase code coverage.
Closes #86